Skip to content

Proper and safe evaluation of realtime capability#4132

Draft
hdiethelm wants to merge 8 commits into
LinuxCNC:masterfrom
hdiethelm:halcmd_getrt
Draft

Proper and safe evaluation of realtime capability#4132
hdiethelm wants to merge 8 commits into
LinuxCNC:masterfrom
hdiethelm:halcmd_getrt

Conversation

@hdiethelm

@hdiethelm hdiethelm commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Intended to be used with:
#4107

Will fix is_sim / is_rt which is broken: #4129

Two new functionality's for two use-cases:

  1. Not running to see if the system is actually capable of running RT
  2. Running to query the real time capability and type

realtime status can be used to check if realtime is running.

The realtime script is extended with the verify command returning 0 if RT capable, 1 if not.
It is intended to use when not running and running.

  • RTAI: Returns always 0
  • uspace: calls rtapi_app getrt and returns the state
    • Not running: rtapi_app performs all the checks and returns immediately
    • Running: rtapi_app calls the master for the real time capability and returns the state

There is the new function hal_get_realtime_type() returning the type of the actually running realtime system trough the hal.

is_sim / is_rt use realtime verify at init.

rtapi_is_realtime() is deprecated: It works only in real time context since #3964 and was never 100% reliable, also according to the doc.

@BsAtHome

BsAtHome commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

I do think it to be very problematic that getting the RT status is so involved. There should be a simple test that does not involve instantiating larger parts of infrastructure.

@hdiethelm

hdiethelm commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

:-D The CI has no realtime:
ERROR MOTION: no realtime detected.

It is somewhat a chicken and egg problem. Not involving the RT infrastructure needs separate code and this can always not be in sync with RT. Involving RT runs a lot of stuff. Let's think about this. Exactly the issue here: #4129

What do you think about a parameter / pin? So you can use getp / gets to ask for realtime?

Or I could add a new command path to halcmd / rtapi_app that does not start the hal if it is not yet running.

@BsAtHome

BsAtHome commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Running non-RT is not an error per se (see CI). Therefore, you cannot and must not "simply" or "blindly" force one or the other.

There are use-cases where you want to know the RT status and that does not always mean that you will or will not be running either. Finding out what the RT status is or will be must be lightweight and may be different from where you ask. Doing it in a component or from the cmd-line may be different, depending how you ask and with what intention you ask.

@grandixximo

grandixximo commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Both CI failures are small:

  • rip-and-test-clang: has_setuid_root is unused on the default uspace build (the detect_* callers are all behind USPACE_RTAI/XENOMAI ifdefs, so the stubs never call it), and -Werror=unused-function kills it. [[maybe_unused]] or moving it inside the ifdefs fixes it.
  • rip-rtai: hal.h:240 extern rtapi_realtime_status_t hal_realtime_status(); needs (void). The RTAI kernel-module build uses -Werror=strict-prototypes, so the empty () fails (clang on the uspace build let it through). Same on the definition in hal_lib.c.

Two runtime things I noticed while reading:

  • rtapi_realtime_status() returns LXRT for detect_rtai(), but makeApp() only handles RTAI, so a uspace RTAI build hits the final else and sets app = nullptr.
  • If can_set_sched_fifo() succeeds but the kernel string matches none of the markers, it falls through to NONE and runs SCHED_OTHER. The old code treated SCHED_FIFO success alone as realtime, so this is a regression on plain-PREEMPT/generic kernels; the fallback there probably wants to stay RT-capable.

Minor: REALTIME_STATUS_PREEMT_* is missing a P (PREEMPT), and detect_preempt_dynamic() tests the same string on both sides of the ||.

@hdiethelm hdiethelm force-pushed the halcmd_getrt branch 2 times, most recently from b73daaf to 5dfd303 Compare June 6, 2026 12:22
@hdiethelm

Copy link
Copy Markdown
Contributor Author

Thanks. Fixed.

One quirk: RTAI in userspace is called LXRT. I should rename this consistently.
https://www.rtai.org/userfiles/documentation/magma/html/api/group__lxrt.html#details

Comment thread src/hal/halmodule.cc Outdated
Comment thread src/hal/halmodule.cc Outdated
Comment thread src/rtapi/rtapi.h Outdated
@hdiethelm hdiethelm force-pushed the halcmd_getrt branch 3 times, most recently from 7f6e60d to fbf2b81 Compare June 6, 2026 13:18
@hdiethelm

Copy link
Copy Markdown
Contributor Author

Hmm, time for a break, to many force pushes, sorry. I will continue tomorrow.

So you are OK with the general concept? Then I will polish it up, update the doc and do some more testing in different combinations.

Open:

  • Python enum
  • Python is_sim / is_rt: I would prefer just removing them
    • Alternative: Dynamic property, but this is also halve breaking due to unknown has to be handled. I could throw an error in this case.

Deferred:

  • rtapi_app start / stop behaivour

@grandixximo

Copy link
Copy Markdown
Contributor

The CI fixes and the LXRT / fallback handling look good now.

On naming, with @BsAtHome's #4099 in mind: rtapi_get_realtime_type() is consistent with the existing rtapi_get_* getters. The hal-side hal_get_realtime_type() is the one I'd reconsider, since #4099 is standardizing hal_get_<datatype>(ref) (e.g. hal_get_si32(ref), hal_get_bool(ref)) where the suffix is a HAL data type and it takes a typed ref. A parameterless hal_get_realtime_type() reads off-pattern in that family; something like hal_realtime_type() would keep hal_get_* reserved for the value accessors. @BsAtHome owns that convention, so up to him.

For the two open how-tos:

  • Dynamic is_rt/is_sim without breaking the API: lib/python/hal.py already wraps _hal (from _hal import *), so a PEP 562 module __getattr__ there gives live values: return get_realtime_type() > 0 for is_rt and <= 0 for is_sim. Existing callers (pncconf, stepconf) keep working; the one change is that before rtapi_app runs the value is -1, so is_rt reads False / is_sim True.
  • Exposing the enum to Python: one PyModule_AddIntConstant(m, "REALTIME_TYPE_NONE", REALTIME_TYPE_NONE) per value, consistent with the existing HAL_BIT etc. constants.

@BsAtHome

BsAtHome commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

something like hal_realtime_type() would keep hal_get_* reserved for the value accessors. @BsAtHome owns that convention, so up to him.

Well, I don't own it. However, I agree with the argument to leave the get/set moniker to the hal pin/param data access.

@hdiethelm

hdiethelm commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

The last few commits are still figuring out ways to solve all issues, not ready for code style review yet.

@grandixximo Thanks a lot for the hints. Helps a lot not to have to search everything.

  • hal_get_realtime_type: There is also hal_get_param_value_by_name and so on, so also consistent.
  • Python enum: Done
  • is_rt / is_sim That one was annoying: First, i need to create a component if there is none yet and a lot of error handling: bdd3ef9 And then I discovered that stepconf.py / stepconf.py need is_sim before realtime is up and running. So roll back and just call realtime verify at init: cdbace2 However, now this is fully backwards compatible, no side effects expected.
  • I left the python is_initialized function from ^ in, might be this is useful. Many functions can only be used if there is already a component. So you can check if there is already one and create one if needed. hal.component_exists() needs also a component to succeed. I tried... ;-)

@hdiethelm hdiethelm force-pushed the halcmd_getrt branch 2 times, most recently from 6477a97 to af73e08 Compare June 6, 2026 22:13
@hdiethelm

Copy link
Copy Markdown
Contributor Author

And of course, after debian package install, the realtime script is not any more in path. And there was also no define for the path where it is. af73e08 adds one.

For scripts, REALTIME=@REALTIME@ is used, so it is available there.

@hdiethelm

Copy link
Copy Markdown
Contributor Author

Due to error handling is annoying not knowing when rtapi_app is running or not:
579f7ed
Let's see how much fall out this generates...

It now works exactly the same as RTAI. realtime start is now needed before every halcmd involving realtime.
Everything else including halrun / linuxcnc usw. works as before what I tested so far.

And it generated a new ToDo: Cleanup the realtime script. It is a mess. There are a lot of RTAI only parts executed in uspace mode. Like loading an empty list of modules and so on. So far, I just added the things I needed.

@grandixximo

Copy link
Copy Markdown
Contributor

Two notes after the latest push.

Naming: you're right, I'll withdraw my concern. hal_get_realtime_type() matches the existing hal_get_lock(), which is already a parameterless global-state getter, so hal_get_* isn't exclusively the typed-ref family. Leave it as is.

CI / auto-start: the two failures (raster, hal-show) come from dropping the uspace auto-start of rtapi_app on first loadrt. raster runs halcmd -f raster.hal (a loadrt with no realtime start) and now gets No master found. That breaks standalone halcmd -f *.hal scripts in general, so I'd suggest not hard-breaking it: keep auto-start working but emit a one-time deprecation warning pointing at realtime start. That keeps existing scripts working and gives a migration path. Then migrate our own scripts/configs/tests to call realtime start explicitly so the tree models the new idiom (and update the expected outputs, since a stderr warning will otherwise trip output-comparison tests like hal-show).

Separately: is dropping the auto-start actually needed for the RT-status goal, or is it a cleanup that could be its own PR? Keeping them decoupled would let the getrt/realtime_type work land without the broader behavior change.

@hdiethelm

Copy link
Copy Markdown
Contributor Author

If these test fails due to a missing realtime start, they most probably fail also with RTAI, i have to test it.

It is not needed in this PR. However, as soon you like to use hal_realtime_type() in many places, it will help a lot, duet to realtime needs to be started before.

I will move it in a separate PR as initialy planned, this one gets already big.

@hdiethelm

hdiethelm commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Option 3 would need some additional code changes to separate SCHED_FIFO from the warning and the reported RT type. Surely possible but if you agree option 2 is the best, this is already implemented.

@BsAtHome Are you also fine with option 2?

Done:

  • Spelling mistakes (Not my strength, I hope I caught all of them...)
  • Doc Python Exceptions
  • Doc HAL status code in hal_get_realtime_type
  • Some retesting after my last change
  • Test on real CNC

Still open:

  • Nothing from my side

@hdiethelm hdiethelm changed the title WIP: Proper and safe evaluation of realtime capability Proper and safe evaluation of realtime capability Jun 14, 2026
@hdiethelm hdiethelm marked this pull request as ready for review June 14, 2026 17:03
Comment thread src/hal/hal_lib.c
Comment thread src/hal/halmodule.cc Outdated
Comment thread src/hal/halmodule.cc Outdated
Comment on lines +2406 to +2411
//Call realtime verify to gather realtime status
//Most probably we don't have realtime running yet
int ret = system(EMC2_REALTIME " verify > /dev/null");
int exit_stat = WEXITSTATUS(ret);
if(exit_stat != 0 && exit_stat != 1){
PyErr_Format(PyExc_RuntimeError, "realtime verify failed, system() return value %i / exit %i", ret, exit_stat);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling system() in the hal module is extremely ugly and a sign that something is amiss.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only alternative I see is to remove is_sim / is_rt and call realtime verify in the two places this was used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the constants properties will allow you to check in a function. The library user will not see the difference.

Question: is is_rt and is_sim called before a component is created?

If there is HAL connection, then you can directly use a pin/param's value exported by the rtapp's inner process.

If there is no connection (hal_init() was not called) then you need to establish a temporary connection (component) so that you can get the values you need.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use properties that call get_realtime_type() successfully, two conditions must be fulfilled.

  • rtapi_app must be running
  • A component must exist

Both are not fulfilled at the two places this is used:


To have is_sim / is_rt useful, it should work always, you should not have to check that you fulfill this conditions. Two options:

  1. Use realtime verify -> In this PR
  2. Make sure both conditions are met

I already implemented 2. before 1. it but discarded it again, mainly to not overgrow this PR into yet another "fix everything" PR: #4132 (comment)

  • rtapi_app must be running
    • This needs a start / stop command
    • Removing autostart / autostop is not really needed but would make sense to avoid inconsistencies
    • Works mostly, some tests fail due to realtime start / realtime stop is missing. They probably will also fail with RTAI.
    • Commit: hdiethelm@612183a
  • A component must exist
    • We can create a temporary one and discard it again
    • Works, needs some cleanup
    • Commit: hdiethelm@cdcc344

Both commits contain also some changes that I found useful anyway and are still in this PR.

At the moment, rtapi_app is started at first load_rt command and exits at exit or after last component has been unloaded. This makes it cumbersome to use get_realtime_type() due to you can do it only after first load_rt, otherwise you will get REALTIME_TYPE_UNINITIALIZED.

RTAI always needs realtime start to before first command, realtime stop at the end, so this is already there. That's why I think it is an option to change from auto to manual start/stop without to much issues.

If you think ^^ is a better solution, I can add it again. It looks in general cleaner to me but it might also break more things.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: Due to it is only used in two places, replacing it by realtime verify and remove is_sim / is_rt would be option 3. However, I don't know who uses this in his own private machine configs.

@hdiethelm hdiethelm Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use properties that call get_realtime_type() successfully, two conditions must be fulfilled.

  • rtapi_app must be running
  • A component must exist

Very true. This is always the case for halmodule to be useful at all.

Only 99%. You can use the hal with only user components and have rtapi_app not running. Not sure why but you can. In this case you probably won't bother to check for RT.

Alternatives may be appropriate here.

If you are ok with (somewhat above in the huge thread):

  • Python is_sim / is_rt: I would prefer just removing them

I just have to find a way to use the realtime script inside this stepconf / pncconf. The check is anyway annoying in there, you are forced to have realtime, no way to "continue anyway". So I will get rid of is_sim / is_rt, use realtime status and fix the check so it is not stopping you by force. Ok?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true. This is always the case for halmodule to be useful at all.

Only 99%. You can use the hal with only user components and have rtapi_app not running. Not sure why but you can. In this case you probably won't bother to check for RT.

And that is the point. When you only use it for non-RT, then why ask about RT? You aren't using RT in non-RT so it is just like "Hey, look, I can run non-RT on RT systems! Am I cool or what?". Right...

Alternatives may be appropriate here.

If you are ok with (somewhat above in the huge thread):
* Python is_sim / is_rt: I would prefer just removing them

Yes, it is not used anywhere else and it is rather uninformative/complicated/etc..

We should just create a RO parameter in rtapi_app (the RT running core) that sets the appropriate value to indicate how the system is running.

I just have to find a way to use the realtime script inside this stepconf / pncconf. The check is anyway annoying in there, you are forced to have realtime, no way to "continue anyway". So I will get rid of is_sim / is_rt, use realtime status and fix the check so it is not stopping you by force. Ok?

Lets try, yes.

Question: is configuration building with stepconf and pncconf different depending on whether it detects running on a RT system? What about cross-configuration (like cross-compiling)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just create a RO parameter in rtapi_app (the RT running core) that sets the appropriate value to indicate how the system is running.

There is no parameter withouth a component.
I have a commit somewhere creating a static component at hal_lib init, but this fails all tests due to there is a new unexpexted component and parameter. But I could add just a simple rt_info.comp you can manually load if needed.

Lets try, yes.

Question: is configuration building with stepconf and pncconf different depending on whether it detects running on a RT system? What about cross-configuration (like cross-compiling)?

It checks RT only to say you are not allowed to do axis tests withouth RT and locks you out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RT components always run in the rtapi_app context. They can use rtapi_*() calls at any time to determine their status.

Userspace components need a HAL connection and could query a HAL parameter. This parameter must be added via a RT component. Other userspace could use halcmd getp.

The question is then do we add this component implicitly or do we need to do so explicitly.

The case for explicit (put it in a .hal file) is there because if you need the info, then you already know that you need the info and add it to the tree of components.
The case for implicit is that you no longer have to think about adding the component and the information is there. If this can be done without that unloading RT becomes a problem (the rmmod problem, I believe), then this may be the easier option for users. Otherwise, explicit must be the way.

For stepconf/pncconf, well, I guess it can spawn a realtime verify and sample the result? Would that suffice? That said, stepconf and pncconf need to build a working .hal file and load/run RT anyway for testing axes. Why not let them load the isrt.comp and sample the information? If it runs in sim-mode, then the programs can disable their controls or tear down rtapi_app again?

@hdiethelm hdiethelm Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With thia PR, you have hal_get_realtime_type() that works in user / rt comps and python. So no real need for a parameter for most usecases I think.

Only issue: This works only after the first load_rt due to rtapi_app is only running afterward. hdiethelm@612183a would fix this.

For stepconf/pncconf

realtime verify would work, except the install path of this scipt needs to be available, i did not find it exposed in python yet.
hal.get_realtime_type() would also work, but only after the first load_rt. No need for a parameter, it will be there anyway only after the first load_rt, implicit or explicit, same issue.

Or I just add the commit above and fix the tests, then hal_get_realtime_type() will work after halrun.

@hdiethelm hdiethelm marked this pull request as draft June 16, 2026 17:57
@hdiethelm hdiethelm force-pushed the halcmd_getrt branch 2 times, most recently from 1605784 to 1799fef Compare June 16, 2026 19:26
@hdiethelm

Copy link
Copy Markdown
Contributor Author

@BsAtHome 1799fef replaces hal.is_sim / hal.is_rt by realtime.verify().
A python wrapper makes it simple to use. I need an .py.in file to get the path. I did not find anywhere in the python part where this path is already available.
It still needs some dok but I think the idea is clear. Do you think this is better? So I will finalize it.

@BsAtHome

Copy link
Copy Markdown
Contributor

Running it with system (a shell) may require you to redirect input from /dev/null too so it cannot hang on waiting for input (unlikely, but you don't need stdin).
Expanding a .py.in file has the advantage that you will get the one singular target script that matches the build. Also in RIP.

Calling the module "realtime" may not be the best of choices and may lead to confusion because the script starting/stopping realtime is called realtime. Maybe the python module should be called something in the direction of lcncrtquery (because you have both a verify and status)? I'm open for other suggestions.

@hdiethelm

Copy link
Copy Markdown
Contributor Author

Running it with system (a shell) may require you to redirect input from /dev/null too so it cannot hang on waiting for input (unlikely, but you don't need stdin).

Stdin is not needed but better save than sorry, right.

Expanding a .py.in file has the advantage that you will get the one singular target script that matches the build. Also in RIP.

Yes, i found out that the realtime script has a special path after a debian package install, so this is the only way to always find it.

Calling the module "realtime" may not be the best of choices and may lead to confusion because the script starting/stopping realtime is called realtime. Maybe the python module should be called something in the direction of lcncrtquery (because you have both a verify and status)? I'm open for other suggestions.

As a wrapper of this script, start / stop / restart / ... could be added to. There was no need to do this now, but i can add all commands. So I think the name is mostly fine. Might be lcncrealtime?

Something like:
realtime.start()
halcmd...
realtime.stop()
is sometimes usefull. It is not needed for uspace (except we move to manual start/stop of rtapi_app), but RTAI needs this.
Unitl now, this is often done in bash around calling python or other binarys or in halrun around calling halcmd.

@hdiethelm

Copy link
Copy Markdown
Contributor Author

@BsAtHome Are you ok with realtime.py or might be lcncrealtime.py?

@BsAtHome

Copy link
Copy Markdown
Contributor

Yes, I do think that lcncrealtime.py would be better. Otherwise we risk confusion with the realtime script. If we ever are to replace the shell script with the python code, then we can simply exec in the old script.

@hdiethelm

hdiethelm commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

After looking at that name to long I went for lcnc_realtime.py. I found it hard to read without underscore. Is this fine to?

This probably would need some doc? Where to put it? The only doc for python code I found is src/config/python-hal-interface.adoc. Or is it good enough as it is documented in the python code right now?

@BsAtHome

Copy link
Copy Markdown
Contributor

After looking at that name to long I went for lcnc_realtime.py. I found it hard to read without underscore. Is this fine to?

This is a module you use in import.I think using an underscore is rather unusual, or not?

This probably would need some doc? Where to put it? The only doc for python code I found is src/config/python-hal-interface.adoc. Or is it good enough as it is documented in the python code right now?

Somewhere... It needs some reorganisation either way. However, this is not a HAL interface, but an RTAPI interface if I look at this right.

@hdiethelm

Copy link
Copy Markdown
Contributor Author

After looking at that name to long I went for lcnc_realtime.py. I found it hard to read without underscore. Is this fine to?

This is a module you use in import.I think using an underscore is rather unusual, or not?

I seldom work in python. But as much as I know you normally create packages, so it would be:
from lcnc import realtime
realtime.verify()

Or:
import lcnc
lcnc.realtime.verify()

This way, you don't take the whole namespace. The way it is now, hal is taken from linuxcnc, so you can not use any other package / class named hal on your system.

Creating a a lcnc package for everything would be an other project.
I can try to start with realtime creating a lcnc package with only this one inside.

This probably would need some doc? Where to put it? The only doc for python code I found is src/config/python-hal-interface.adoc. Or is it good enough as it is documented in the python code right now?

Somewhere... It needs some reorganisation either way. However, this is not a HAL interface, but an RTAPI interface if I look at this right.

So src/config/lcnc_realtime.adoc?

This helps to check if hal functions can be used or if a component needs
to be created first.
…_app

Python is_rt / is_sim now use "realtime verify" which uses
"rtapi_app check_rt" in uspace / returns true in rtai.

If realtime not running: rtapi_app performs the checks an returns the
state

If realtime running: rtapi_app calls master to perform the check and
returns the state
The same checks are performed always the same now. If something is not
properly checked, makeApp() fill fail instead of just chosing a
different RT implementation by itsself.

New function: rtapi_realtime_type_t rtapi_get_realtime_type(void)
rtapi_is_realtime() was always unreliable and broken for user components
for some time. It is fixed but only available for rt components now.

hal_get_realtime_type() returns the true running realtime type
through the HAL for user and realtime components.

This function is also exposed through python hal.

rtapi_app now unloads hal_lib at exit, so everything is cleaned up
properly and realtime_type is set back to REALTIME_TYPE_UNINITIALIZED.
REALTIME_TYPE_UNKNOWN / REALTIME_TYPE_PREEMPT_DYNAMIC need
LINUXCNC_FORCE_REALTIME=1 to be set.

These two options are most likely not desired and should not be selected
automatically.
lcnc_realtime.verify() must be used instead of hal.is_rt
@hdiethelm

Copy link
Copy Markdown
Contributor Author

Rebased to master and created a package.

Now you can use:

from lcnc import realtime

print("realtime.verify " + str(realtime.verify()))
print("realtime.status " + str(realtime.status()))

or

import lcnc

print("realtime.verify " + str(lcnc.realtime.verify()))
print("realtime.status " + str(lcnc.realtime.status()))

I am seeing quite a lot of random files from linuxcnc installed just in top of dist-packages:

/usr/lib/python3/dist-packages/raster.py
/usr/lib/python3/dist-packages/preview_helpers.py
/usr/lib/python3/dist-packages/propertywindow.py
/usr/lib/python3/dist-packages/pyhal.py
/usr/lib/python3/dist-packages/pyngcgui.py
/usr/lib/python3/dist-packages/hal.py
/usr/lib/python3/dist-packages/hal_glib.py
/usr/lib/python3/dist-packages/hershey.py
/usr/lib/python3/dist-packages/lineardeltakins.cpython-313-x86_64-linux-gnu.so
/usr/lib/python3/dist-packages/linux_event.py
/usr/lib/python3/dist-packages/linuxcnc.cpython-313-x86_64-linux-gnu.so
/usr/lib/python3/dist-packages/linuxcnc_util.py
/usr/lib/python3/dist-packages/multifilebuilder.py
/usr/lib/python3/dist-packages/common
/usr/lib/python3/dist-packages/common/colored_formatter.py
/usr/lib/python3/dist-packages/common/hal_glib.py
/usr/lib/python3/dist-packages/common/iniinfo.py
/usr/lib/python3/dist-packages/common/logger.py
...

^^ looks to me like really bad practice, even if I have basically no clue of python.
It would probably make sense to move them all to a lcnc (or linuxcnc?) python package to not collide with other packages now or in the future. However, that's a whole new project fixing all the imports if you do that.

@grandixximo Do you work more with python? Any insight? Makes my package sens the way I am doing it?

At least after installing the deb, I can run the above code directly with python and stepconf also passes the realtime check, so it is can not be to far... ;-)

Otherwise, I could also just roll back the package creation and stick to lcnc_realtime.py / realtime.py instead of starting a whole new concept of creating a proper linuxcnc python package.

It is not worse than the existing code. realtime.py will never collide with a python implementation of the realtime script, due to it will live in scripts/ or bin/ in the source and under /usr/lib/linuxcnc/realtime after install. The realtime.py module lives under lib/python/realtime.py and under /usr/lib/python3/dist-packages/realtime.py after install.

@grandixximo

Copy link
Copy Markdown
Contributor

Your instinct on the pollution is right: SITEPY is the global /usr/lib/python3/dist-packages, and we dump ~30 generically-named flat modules (hal.py, gremlin.py, nf.py, raster.py) plus generic dirs (common/, touchy/) straight into the shared namespace. A single branded package is the correct long-term fix, and a bare realtime.py there would be a real collision hazard, so you're not wrong to want a package.

That said, for this PR I'd go with the flat lcnc_realtime.py and not the package. Two reasons:

  1. The package makes a project-wide namespace decision inside a realtime-status PR. "What is LinuxCNC's official python namespace" deserves its own thread, the naming alone already took a week here.
  2. We already carry three brand names: emc2/emc in the build and env layer (EMC2_HOME, EMC2VERSION, and the EMC2_REALTIME define this very PR adds), linuxcnc in the package and user layer, and the linuxcnc C extension .so. lcnc would be a fourth. So picking a python namespace is really a chance to consolidate, not just add one more name, and that consolidation shouldn't be decided by default in a status PR. The cleanest target is linuxcnc, but the .so squats it, and turning that into a linuxcnc/__init__ + inner .so is the real fix, which is exactly the bigger project you don't want to take on here. Starting lcnc now risks a second migration later.

So: ship lcnc_realtime.py (flat, collision-safe name, matches today's convention, unblocks this and #4107), and open a separate PR/issue for a proper namespace where the dist-packages cleanup gets the attention it deserves, ideally aimed at linuxcnc. If you and @BsAtHome would rather land the package now, I'm fine with it too, just get an explicit "lcnc is our namespace going forward" on record first so it isn't decided by default.

One thing that matters more than the packaging: hal.is_sim / hal.is_rt were documented public attributes (in python-hal-interface.adoc, which this PR deletes). Hard-removing them breaks any external user HAL/python config or custom GUI reading them. I'd keep a thin deprecation shim, the names computed via the new path with a warning, rather than removing them outright.

And if the package does stay: drop the eager from . import realtime in __init__.py so import lcnc doesn't pull in every future submodule; use subprocess.run([REALTIME, "verify"], stdin=DEVNULL, stdout=DEVNULL) instead of os.system (no shell, covers BsAtHome's stdin point); README is missing a trailing newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants